Skip to content

at86rf2xx/Check at86rf2xx state before changing phy#7207

Closed
biboc wants to merge 1 commit intoRIOT-OS:masterfrom
biboc:at86rf2xx/Fix_saving_unstable_state
Closed

at86rf2xx/Check at86rf2xx state before changing phy#7207
biboc wants to merge 1 commit intoRIOT-OS:masterfrom
biboc:at86rf2xx/Fix_saving_unstable_state

Conversation

@biboc
Copy link
Member

@biboc biboc commented Jun 19, 2017

at86rf2xx: Changing phy involve to save current transceiver state, set transceiver to OFF and set it back again to its previous state. This PR avoids to save transceiver state AT86RF2XX_STATE_IN_PROGRESS and wait that transceiver is in stable state

@biboc biboc requested a review from jnohlgard June 19, 2017 15:36
@biboc biboc force-pushed the at86rf2xx/Fix_saving_unstable_state branch from 44a4e76 to f3df570 Compare June 19, 2017 15:38
Copy link
Member

@thomaseichinger thomaseichinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@biboc Could you please elaborate on what this change is actually fixing? Is there a bug associated with this?

Also please give this PR a descriptive title.

@biboc
Copy link
Member Author

biboc commented Jun 19, 2017

Yes when trying to change channel with OpenThread, at86rf2xx_configure_phy is called.
The driver saves current state, set state to AT86RF2XX_STATE_TRX_OFF, configure PHY and re-set latest state.
Problem is that we can't change at86rf2xx state if it is in AT86RF2XX_STATE_IN_PROGRESS state so with this PR we make sure transceiver is in the right state before changing it and also, we make sure that we do try to set the transceiver to AT86RF2XX_STATE_IN_PROGRESS at the end of at86rf2xx_configure_phy.
Without this fix, the driver loops forever.

@biboc biboc changed the title at86rf2xx/Fix_saving_unstable_state at86rf2xx/Fix_do_not_save_AT86RF2XX_STATE_IN_PROGRESS_state_before_changing_phy Jun 19, 2017
@biboc biboc changed the title at86rf2xx/Fix_do_not_save_AT86RF2XX_STATE_IN_PROGRESS_state_before_changing_phy at86rf2xx/Do not save AT86RF2XX_STATE_IN_PROGRESS state before changing phy Jun 19, 2017
@biboc biboc changed the title at86rf2xx/Do not save AT86RF2XX_STATE_IN_PROGRESS state before changing phy at86rf2xx/Check at86rf2xx state before changing phy Jun 19, 2017
@biboc
Copy link
Member Author

biboc commented Jun 19, 2017

@thomaseichinger, better?

_set_state(dev, AT86RF2XX_STATE_TRX_OFF, state);
return;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see how moving this block helps. at86rf2xx_configure_phy uses AT86RF2XX_STATE_TRX_OFF not AT86RF2XX_STATE_FORCE_TRX_OFF thus it shouldn't have an effect.

@thomaseichinger
Copy link
Member

@biboc Thanks for changing the title.

Could you help me understand by telling me which loop the driver gets stuck in?

@DipSwitch
Copy link
Member

DipSwitch commented Jun 19, 2017 via email

@thomaseichinger
Copy link
Member

@biboc Could you please check if #7115 solves your problem?

@biboc
Copy link
Member Author

biboc commented Jun 20, 2017

AT86RF212B Datasheet Extract : "Do not try to initiate a further state change while the radio transceiver is in STATE_TRANSITION_IN_PROGRESS state. "

During a channel setting we use :
void at86rf2xx_set_chan(at86rf2xx_t *dev, uint8_t channel) :
which calls :
void at86rf2xx_configure_phy(at86rf2xx_t *dev)
which calls :
void at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state)
which calls :
static inline void _set_state(at86rf2xx_t *dev, uint8_t state, uint8_t cmd)
{
at86rf2xx_reg_write(dev, AT86RF2XX_REG__TRX_STATE, cmd);

/* To prevent a possible race condition when changing to
* RX_AACK_ON state the state doesn't get read back in that
* case. See discussion
* in #5244
*/
if (state != AT86RF2XX_STATE_RX_AACK_ON) {
while (at86rf2xx_get_status(dev) != state);
}

dev->state = state;
}
We get stuck in the while sometimes. Because in
void at86rf2xx_configure_phy(at86rf2xx_t *dev)
we store the current state, to restore it after phy configuration
but unfortunately in my case we save AT86RF2XX_STATE_IN_PROGRESS instead of a real state.
When this "state" is restored at the end of phy configuration, we get stuck in : static inline void _set_state(at86rf2xx_t dev, uint8_t state, uint8_t cmd)
Because the transceiver can't never reach AT86RF2XX_STATE_IN_PROGRESS state. In my case the transceiver only reach the AT86RF2XX_STATE_TRX_OFF state.
That's why before any state change we need to check that we are NOT in AT86RF2XX_STATE_IN_PROGRESS.
That's why I also inverted those two blocks to prevent any freeze:
/
make sure there is no ongoing transmission, or state transition already
* in progress */
while (old_state == AT86RF2XX_STATE_BUSY_RX_AACK ||
old_state == AT86RF2XX_STATE_BUSY_TX_ARET ||
old_state == AT86RF2XX_STATE_IN_PROGRESS) {
old_state = at86rf2xx_get_status(dev);
}

if (state == AT86RF2XX_STATE_FORCE_TRX_OFF) {
_set_state(dev, AT86RF2XX_STATE_TRX_OFF, state);
return;
}

Blocks inversion should not have any effect.
I agree with you if the transceiver has the same behavior, it should not be freeze because it will reach the AT86RF2XX_STATE_TRX_OFF state. According to the datasheet, it is better to prevention so no weird behaviour can happen.

#7115 answers to the second change (inverting the two blocks in get/set file) but not to my first problem "update phy".

Is it clearer @thomaseichinger ?

@thomaseichinger
Copy link
Member

@biboc Thanks for your explanation.

I think however that #7115 actually also handles the first part of your problem because it waits for the module to be in a "reachable" state (no busy or transition state) and returns the state the hardware is in just after that.

The problem is two fold, (i) trying to return to an unreachable state and (ii) a race between getting the state and then setting it. (i) is handled by waiting and only return reachable states and (ii) should be handled by storing the state the module is in just after assuring (i).

With those changes we can actually remove some of the extra checks before calling at86rf2xx_configure_phy as you can see in DipSwitch#6.

Unless I am totally missing something, I'd appreciate if you could test #7115 (including DipSwitch#6) and see if that fixes your problem.

@biboc
Copy link
Member Author

biboc commented Jun 20, 2017

Merge DipSwitch#6 inside #7115 and I'll try

@biboc
Copy link
Member Author

biboc commented Jun 21, 2017

We've tested your branch @thomaseichinger, it works! Apart from a missing definition in a .h so I close this PR and let you merge the other PR with your changes

@biboc biboc closed this Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants